feat(hogql): cohort left join conjoined #19725

Gilbert09 · 2024-01-11T17:14:47Z

Problem

We were having an issue with big clients using multiple cohort breakdowns. We'd run multiple sequential queries, one for each cohort breakdown due to the breakdown value not getting returned from the subquery cohort method.
Introduced here feat: added trends breakdown by cohort #18028 with a bit more detail on why it was built this way

Changes

Adds a new cohort modifier, leftjoin_conjoined. Built very similar to the leftjoin modifier, but instead of having a LEFT JOIN for each cohort breakdown, we instead have a single LEFT JOIN for all cohorts
This allows us to return the cohort id as the breakdown value in the trends query when the modifier is turned on
The new cohort modifier logic gets ran before the resolver is ran in hogql land. This is because we reference the join in the trends AST and so the left join needs to be added before types get resolved.
This PR does not include turning on the modifier in any way. This is to come in a follow-up PR. Question: how do we want to conditionally turn on modifiers?!

How did you test this code?

Added unit tests / snapshots of the new cohort resolver

github-actions · 2024-01-11T17:23:52Z

Size Change: 0 B

Total Size: 2 MB

ℹ️ View Unchanged

Filename	Size
`frontend/dist/toolbar.js`	2 MB

_{compressed-size-action}

posthog-bot · 2024-01-12T13:20:10Z

Hey @Gilbert09! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

# Conflicts: # mypy-baseline.txt # posthog/hogql_queries/insights/trends/breakdown.py # posthog/hogql_queries/insights/trends/trends_query_runner.py

mariusandra

jumping to a call, dumping thoughts

mariusandra · 2024-01-15T12:16:17Z

posthog/hogql/functions/cohort.py

+    if (isinstance(arg.value, int) or isinstance(arg.value, float)) and not isinstance(arg.value, bool):
+        cohorts = Cohort.objects.filter(id=int(arg.value), team_id=context.team_id).values_list(
+            "id", "is_static", "name"
+        )


Why the float? It seems like some fields in the schema didn't come with /** @asType integer */

The whole breakdown value type union is pretty messed up. Would love to break it down some more into multiple fields with better typing. I've had to use # type: ignore in too many places due to this

mariusandra · 2024-01-15T12:25:51Z

posthog/hogql/transforms/in_cohort.py

+            if not isinstance(arg, ast.Constant):
+                raise HogQLException("IN COHORT only works with constant arguments", node=arg)
+
+            if (isinstance(arg.value, int) or isinstance(arg.value, float)) and not isinstance(arg.value, bool):


same float question here

mariusandra · 2024-01-15T12:27:55Z

posthog/hogql/transforms/in_cohort.py

+                """
+                    SELECT person_id AS cohort_person_id, 1 AS matched, cohort_id
+                    FROM static_cohort_people
+                    WHERE {cohort_clause}
+                    UNION ALL
+                    SELECT person_id AS cohort_person_id, 1 AS matched, cohort_id
+                    FROM raw_cohort_people
+                    WHERE {cohort_clause}
+                    GROUP BY cohort_person_id, cohort_id, version
+                    HAVING sum(sign) > 0
+                """,


Should we figure out which ones are static and which are dynamic, and remove one side of this union when needed? I think this could save anywhere between a bit and a bunch of CPU cycles.

Yeah, we can check if we're querying just static, just dynamic, or both for sure

mariusandra

I think this makes sense. I see now as well why multiple joins with an OR wouldn't have really worked. Thinking now, there probably would have been a way to get it working with array joins, but it'd then still make multiple queries to the same cohorts tables, and possibly cause other issues with other aggregations. So LGTM, but I'd still see if there's something to do about the typing int/float typing. Can probably be later as well.

Edit: also there are some failing tests.

neilkakkar · 2024-01-15T14:58:28Z

posthog/hogql/transforms/test/__snapshots__/test_in_cohort.ambr

+  FROM cohortpeople 
+  WHERE and(equals(cohortpeople.team_id, 420), in(cohortpeople.cohort_id, [1])) 
+  GROUP BY cohort_person_id, cohortpeople.cohort_id, cohortpeople.version 
+  HAVING ifNull(greater(sum(cohortpeople.sign), 0), 0)) AS __in_cohort ON equals(__in_cohort.cohort_person_id, events.person_id) 


flyby: cohortpeople.sign is deprecated, shouldn't be used, this doesn't work properly here. Rather, we look at the version, if it's equal to the version on the cohort.

I see sign is still used in a few places for cohort calculations, but I think those places don't face this same issue 🙈 (for various reasons). In querying though, stick to version!

This means somehow passing the current cohort version value to the query generator

basically, the problem here is some stale versions of the cohorts might have sum(sign) > 0 as well, which can then lead to extra persons being filtered, iirc.

@neilkakkar Cool, so, I need to grab cohort.version and filter cohort_people + removing the sign clause? 👌

Yep, should work well!

This means you can get rid of the group by too, which should speed things up a lot, just copy this query:

https://github.com/PostHog/posthog/blob/master/posthog/models/cohort/sql.py#L75

Dreamy, have added this in, but need to remove the group by. Thanks!

WIP

d555c56

Added a new cohort modifier for a conjoined left query

e1b926c

Gilbert09 changed the title ~~WIP~~ feat(hogql): cohort left join conjoined Jan 12, 2024

Gilbert09 requested review from mariusandra and a team January 12, 2024 13:19

Gilbert09 marked this pull request as ready for review January 12, 2024 13:20

Gilbert09 and others added 5 commits January 12, 2024 13:20

Merge branch 'master' into tom/trends-cohort-breakdowns

fea3695

Missing changes

5a999ca

Regenned mypy sync file

b1c4dc5

Fixed tests

cf5306d

Update query snapshots

5536b6d

thmsobrmlr force-pushed the tom/trends-cohort-breakdowns branch from 8a9b626 to 7db98bb Compare January 15, 2024 12:07

Merge branch 'master' into tom/trends-cohort-breakdowns

9265096

# Conflicts: # mypy-baseline.txt # posthog/hogql_queries/insights/trends/breakdown.py # posthog/hogql_queries/insights/trends/trends_query_runner.py

thmsobrmlr force-pushed the tom/trends-cohort-breakdowns branch from 7db98bb to 9265096 Compare January 15, 2024 12:08

mariusandra reviewed Jan 15, 2024

View reviewed changes

mariusandra approved these changes Jan 15, 2024

View reviewed changes

neilkakkar reviewed Jan 15, 2024

View reviewed changes

Gilbert09 and others added 10 commits January 15, 2024 15:09

Seperate out dynamic and static cohorts

c38325b

Use version when filtering dynamic cohorts

1880ba5

Fixed tests

64a7653

Update query snapshots

4b545c0

Updated mypy baseline

e99429a

Removed group by

e368288

Update query snapshots

1756571

Update query snapshots

27d1e50

Update query snapshots

dc2bb1b

Merge branch 'master' into tom/trends-cohort-breakdowns

5b74088

Gilbert09 merged commit 626e880 into master Jan 16, 2024
99 checks passed

Gilbert09 deleted the tom/trends-cohort-breakdowns branch January 16, 2024 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hogql): cohort left join conjoined #19725

feat(hogql): cohort left join conjoined #19725

Gilbert09 commented Jan 11, 2024 •

edited

Loading

github-actions bot commented Jan 11, 2024 •

edited

Loading

posthog-bot commented Jan 12, 2024

mariusandra left a comment

mariusandra Jan 15, 2024

Gilbert09 Jan 15, 2024

mariusandra Jan 15, 2024

mariusandra Jan 15, 2024

Gilbert09 Jan 15, 2024

mariusandra left a comment •

edited

Loading

neilkakkar Jan 15, 2024

neilkakkar Jan 15, 2024

Gilbert09 Jan 15, 2024

neilkakkar Jan 15, 2024

Gilbert09 Jan 15, 2024

feat(hogql): cohort left join conjoined #19725

feat(hogql): cohort left join conjoined #19725

Conversation

Gilbert09 commented Jan 11, 2024 • edited Loading

Problem

Changes

How did you test this code?

github-actions bot commented Jan 11, 2024 • edited Loading

posthog-bot commented Jan 12, 2024

mariusandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusandra left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gilbert09 commented Jan 11, 2024 •

edited

Loading

github-actions bot commented Jan 11, 2024 •

edited

Loading

mariusandra left a comment •

edited

Loading